fix(command): avoid duplicate GEOSEARCH args - #3955
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5fcd35960
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| baseCmd: baseCmd{ | ||
| ctx: ctx, | ||
| args: geoSearchLocationArgs(opt, args), | ||
| args: args, |
There was a problem hiding this comment.
This leaves the exported constructor no longer expanding GeoSearchLocationQuery into the GEOSEARCH arguments. Code that constructs this command directly, such as NewGeoSearchLocationCmd(ctx, q, "geosearch", key) for Client.Process, now sends only the supplied prefix and omits the FROM*/BY*/WITH* options, so Redis returns a syntax or arity error even though the same call worked before; the duplicate-args fix can be applied in the cmdable wrapper without changing the constructor's public behavior.
Useful? React with 👍 / 👎.
|
Hello @mehmettokgoz, thank you for the contribution. I will check this next week, I do see you think the bot is wrong, let me verify that and I will get back to you. |
|
Hey @mehmettokgoz , thanks again for the contribution. I reviewed your PR, and you're correct that the arguments are being sent twice. To preserve the expected behavior of the public constructor |
3 participants
GeoSearchLocationbuilds the complete command argument list ingeo_commands.goand then passes it toNewGeoSearchLocationCmd.The problem was in
NewGeoSearchLocationCmdincommand.go. Instead of storing the complete argument list it received, the constructor calledgeoSearchLocationArgsagain. This appended the same query options a second time:Update
NewGeoSearchLocationCmdto preserve the supplied arguments so that each option is included only once.Note
Low Risk
Small constructor fix for geo commands with a regression test on argument encoding; no auth or data-path changes.
Overview
Fixes GEOSEARCH commands built by
GeoSearchLocationso Redis receives each query option once instead of twice.NewGeoSearchLocationCmdno longer re-runsgeoSearchLocationArgson the args already assembled ingeo_commands.go; it stores the passed-in slice as-is. A test now assertscmd.Args()for a fullGeoSearchLocationquery (fromlonlat, byradius, sort, withcoord/withdist/withhash).Reviewed by Cursor Bugbot for commit f5fcd35. Bugbot is set up for automated code reviews on this repo. Configure here.